Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a fix for incomplete add-url functionality (related to PR #185 and issue #141). The changes include significant refactoring and new features:
Changes:
- Adds pre-commit cache system for tracking LFS file metadata locally
- Reorganizes cloud/S3 utilities from
s3_utilstocloudpackage - Updates import paths for data-client package structure changes
- Implements new LFS tracking using git check-attr instead of go-git library
- Adds extensive test coverage for new features
Reviewed changes
Copilot reviewed 80 out of 85 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds local replace directive for data-client dependency |
| cmd/precommit/ | New pre-commit hook implementation for cache updates |
| cmd/prepush/ | Refactored to use pre-commit cache for performance |
| cmd/addurl/ | Major refactor with new service pattern and cache integration |
| precommit_cache/ | New package for read-only cache access |
| cloud/ | Renamed from s3_utils with new S3 inspection and download helpers |
| lfs/ | Refactored LFS tracking to use git check-attr |
| utils/ | Added helper functions and moved LFS tracking utilities |
| tests/ | Updated integration tests and added new test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Implemented fix for #198 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 89 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matthewpeterkort
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
Alot of duplicated / cleanup needed. I can do this myself since you're off today.
| ) | ||
|
|
||
| // downloads a file to a specified path using a signed URL | ||
| func DownloadSignedUrl(signedURL string, dstPath string) error { |
There was a problem hiding this comment.
This function is used in a lot of places I would have expected there to be a utility function for this in data-client.
There was a problem hiding this comment.
data client was focused on gen3 and has a lot of legacy code. This is pure s3, I'd like to enhance it with https://pkg.go.dev/gocloud.dev/blob when we get a chance. Plus, this use case is focused on foreign urls.
There was a problem hiding this comment.
Plus with #180 this will probably be deferred until we get dynamic buckets on the server
There was a problem hiding this comment.
I'll look into it today
* clean up repo to address my PR feedbacks * remove unused commands * fix:git-parsing #201 * filter out integration tests * filter out integration test * merge url-3-pr-issues * nil logger check --------- Co-authored-by: Brian Walsh <brian@bwalsh.com>
2 participants
Continuation of discussion from #185